Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Allow Irrefutable Patterns in if-let and while-let statements #2086

Merged
merged 11 commits into from
Sep 11, 2017
Merged

RFC: Allow Irrefutable Patterns in if-let and while-let statements #2086

merged 11 commits into from
Sep 11, 2017

Conversation

Nokel81
Copy link
Contributor

@Nokel81 Nokel81 commented Jul 27, 2017

This is an RFC for allowing irrefutable patterns in if-let and while-let statements

Edit: Rendered

[updated to link to final rendered version]

@eddyb
Copy link
Member

eddyb commented Jul 27, 2017

@Nokel81 The title of the PR should be the RFC name (you can edit it).

@Nokel81 Nokel81 changed the title Inital RFC commit RFC: Allow Irrefutable Patterns in if-let and while-let statements Jul 27, 2017
@main--
Copy link

main-- commented Jul 27, 2017

It allows programmers to manually write the line if let _ = expr { } else { } which is generally obfuscating and not desirable. However, a different RFC should solve this.

I thoroughly disagree. This is already solved right now - the code is wrong and thus an error. You're proposing a different solution which has to deal with this.

If anything, this is an unresolved question: If we can't solve it by making it an error, how do we solve it?

This RFC is also missing its Alternatives section even though I can think of two options off the top of my head:

  • The trivial alternative: Do nothing. As your motivation explains, this only matters for macros anyways plus there already is an acceptable workaround (match). Code that needs this frequently can just package this workaround in its own macro and be done.
  • Turn the error into a lint that errors by default. Unreachable match arms are usually wrong except in some macros and that's why you can disable the warning there with #[allow(unreachable_patterns)]. The same goes for irrefutable if/while-let patterns, so it only seems natural to apply a similar solution.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

I will add those alternatives. From what I understand #[allow] is not allowed infront of if statements

@main--
Copy link

main-- commented Jul 27, 2017

Indeed it is not but perhaps that could be changed?

In any case, you could always just add the attribute to a surrounding block.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

I would say that it should be changed since from what I understand it is unique to if since while allows it. Also the warning should be equivalent to the one for #2087 since it is essentially also a tautology.

@scottmcm scottmcm added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jul 27, 2017
@burdges
Copy link

burdges commented Jul 27, 2017

I think if cfg! should work sensibly, and similarly for the patterns given to macros that you worry about here, but an obviously irrefutable pattern like if let (a,b) = foo() { .. } should be written { let (a,b) = foo(); .. } instead.

Would it work if there were a way to declare an irrefutable pattern refutable? #![ensure_refutable] perhaps?

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 27, 2017

So instead of a warning, keep it a error but still have an [#allow] for it?

@nrc
Copy link
Member

nrc commented Aug 1, 2017

I'd be in favour of the lint option here - seems reasonable for macros to #[allow] a statement to opt-in to this behaviour. Technically this doesn't work for expressions, but I think that just means the macro needs to use a temporary variable.

@joshtriplett
Copy link
Member

I likewise prefer the solution of making this an error-by-default lint that macros can turn off. For non-macro code, errors like this help Rust users (especially new users) learn and avoid using suboptimal forms that just happen to work. And macros can easily turn the lint off.

@tikue
Copy link
Contributor

tikue commented Aug 2, 2017

I am definitely in favor of some way to relax this error. I've stumbled on this in macros, and existing workarounds are not great. It's worth pointing out that match is not a drop-in solution to this RFC because in macros you can't always determine if there should be a _ => ... arm or not; this comes up when dealing with enums with an unknown number of variants. For such cases, the only solution right now (that I've found) is to add a NotIrrefutable variant. Pretty hacky. I didn't know about #[allow(unreachable_patterns)]. That's not so bad actually, but not very discoverable right now.

@joshtriplett
Copy link
Member

@Nokel81 In the @rust-lang/lang meeting, we were all in favor of the solution of making this an error-by-default lint. Could you please adjust the RFC to that effect, and then we'd be inclined to accept it?

@Nokel81
Copy link
Contributor Author

Nokel81 commented Aug 3, 2017

Yup, done. I hope the wording is to your liking

@joshtriplett
Copy link
Member

@Nokel81 Thanks for the fast response!

What's there looks reasonable, but it would help if the summary section had an explanation of the proposed change and reason (e.g. "This can break macros that want to accept patterns generically; this RFC proposes changing this to an error-by-default lint that can be disabled by such macros."). Also, in the detailed design section, please come up with a proposed name for the lint (e.g. irrefutable_conditional_pattern), and write an example of some code using it that the compiler should accept.

…A proposed lint name and some code examples
@Nokel81
Copy link
Contributor Author

Nokel81 commented Aug 4, 2017

How is this?

@joshtriplett
Copy link
Member

joshtriplett commented Aug 4, 2017

@Nokel81 The verbiage looks great. One minor nit: lint names use underscores, not dashes, so can you s/irrefutable-let-pattern/irrefutable_let_pattern/g? With that, I think this will be ready for review.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Aug 4, 2017

Done

@aturon
Copy link
Member

aturon commented Aug 4, 2017

Thanks @Nokel81 and @joshtriplett!

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 4, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Aug 4, 2017
@Nokel81
Copy link
Contributor Author

Nokel81 commented Aug 4, 2017

@aturon Shouldn't S-waiting-on-author be removed?

@durka
Copy link
Contributor

durka commented Aug 30, 2017

As a minor note, I don't like the lint name, because a "let pattern" (i.e. Foo { x, y } in let Foo { x, y }: Foo = make_foo();) already is, and must be, irrefutable. But I don't really have a better suggestion and it can be bikeshedded later.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Aug 30, 2017

The reason that I went with this name is because the current error is "irrefutable if-let pattern". We could change the lint but then what about for while loops (though that may be out of the scope of this RFC)

@yodaldevoid
Copy link

yodaldevoid commented Aug 31, 2017

I have a minor nit with the RFC as written. Under the "Motivation" section the following phrase makes no sense to me:

... to be both much larger and include a compiler allow.

I'm not sure what this is actually trying to say, really, but perhaps it could be reworded?

if let $p = $val {
$b
}
```
Copy link

@ExpHP ExpHP Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two adjacent code blocks are confusing, especially since the second one starts a new thought.

I picture something more along the lines of:

Currently, one must write

(first example)

rather than the more natural syntax

(second example)

because reasons.

[summary]: #summary

Currently when using an if let statement and an irrefutable pattern (read always match) is used the compiler complains with an `E0162: irrefutable if-let pattern`.
The current state breaks macros who want to accept patterns generically and this RFC proposes changing this error to an error-by-default which is allowed to be disabled by such macros.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error-by-default lint?

Changed the flow of the motivation section so as to better distinguish the code examples.
Added the word 'lint' as it was missing from the summary section
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 9, 2017

The final comment period is now complete.

@aturon aturon merged commit f192404 into rust-lang:master Sep 11, 2017
@aturon
Copy link
Member

aturon commented Sep 11, 2017

This RFC has been merged! Tracking issue.

Thanks @Nokel81!

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 11, 2017

Thank you

@Nokel81 Nokel81 deleted the allow_irrefutable-ifwhile-let branch September 11, 2017 21:49
@vitiral
Copy link

vitiral commented Sep 13, 2017

@Nokel81 the rendered link is now broken

@Nokel81 Nokel81 restored the allow_irrefutable-ifwhile-let branch September 13, 2017 23:52
@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 14, 2017

@vitiral fixed

@SoniEx2 SoniEx2 mentioned this pull request Jan 17, 2018
bors added a commit to rust-lang/rust that referenced this pull request Jun 26, 2018
…matsakis

Implementation of RFC 2086 - Allow Irrefutable Let patterns

This is the set of changes for RFC2086. Tracking issue #44495. Rendered [here](rust-lang/rfcs#2086)
@Centril Centril added A-patterns Pattern matching related proposals & ideas A-exhaustiveness Proposals relating to exhaustiveness of pattern matching. A-control-flow Proposals relating to control flow. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-control-flow Proposals relating to control flow. A-exhaustiveness Proposals relating to exhaustiveness of pattern matching. A-patterns Pattern matching related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.